ci: guard that every bundled SKILL.md carries upstream provenance#41
Merged
Conversation
Add a provenance-presence check to validate-manifests.sh: every plugins/*/skills/*/SKILL.md must carry a non-empty metadata.github-repo in its YAML frontmatter (as gh skill install records). AGENTS.md forbids hand-authored/divergent bundled skills, but CI only enforced structural parity (manifest <-> filesystem <-> README) — a hand-edited or provenance-stripped skill passed. The guard stays jq/grep-only (no yq). Pin it in validate-manifests.test.sh: the fixture SKILL.md now carries provenance (happy path), plus three FAIL cases (stripped provenance, no frontmatter, empty github-repo). Update AGENTS.md to document the check. Fixes #40 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a provenance check to ChangesSkill provenance guard
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/validate-manifests.sh`:
- Around line 208-211: The provenance check in validate-manifests.sh is too
loose because it greps any github-repo line and accepts empty/commented values,
so tighten the logic to validate the metadata.github-repo field specifically.
Update the existing manifest validation block around the provenance check to
parse the actual metadata section and require a real non-empty value, rather
than matching top-level github-repo or whitespace/comment-only entries. Keep the
change localized to the skill-provenance guard in scripts/validate-manifests.sh
so it remains the source of truth.
In `@scripts/validate-manifests.test.sh`:
- Around line 200-230: The provenance regression coverage in
validate-manifests.test.sh is incomplete: it only checks missing frontmatter,
missing github-repo, and empty key values, but not misplaced or quoted-empty
provenance. Add failing fixtures around the existing bundled SKILL.md provenance
checks to assert that github-repo outside metadata is rejected and that
github-repo: "" or comment-only values are also rejected, using the same
check_fail pattern so the validate-manifests guard stays pinned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7843d2d5-fc80-4d5e-a095-a0d759e3ae41
📒 Files selected for processing (3)
AGENTS.mdscripts/validate-manifests.shscripts/validate-manifests.test.sh
…ment values Address CodeRabbit review on #41: the grep-based provenance check accepted a top-level github-repo: (outside metadata:) and treated github-repo: "" / github-repo: # comment as non-empty, so a provenance-stripped hand edit could still pass. Replace it with a single awk pass that slices the frontmatter, scopes the lookup to the metadata: block, and rejects empty, quoted-empty (""/''), and comment-only (# …) values. Stays jq/grep-free (no yq). Pin the tightening with three new self-test FAIL fixtures: top-level github-repo, quoted-empty, and comment-only. Self-test 27/0; live guard exit 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #40— Part of #38 (Theme 4: provenance & supply-chain hardening).Problem
AGENTS.mdis emphatic that bundled skills come only from upstream viagh skill installand are never hand-edited to diverge — each install records the true upstream in the skill'smetadata.github-*frontmatter. Butscripts/validate-manifests.shenforced only structural parity (manifest ↔ filesystem ↔ README table). It did not assert that every bundledSKILL.mdactually carries that provenance, so a hand-authored or provenance-stripped skill passed CI today — silently breaking the "sourced from upstream, never divergent" contract.Change
validate-manifests.shgainsvalidate_skill_provenance(): for everyplugins/*/skills/*/SKILL.md, it slices the YAML frontmatter (the block between the first two---lines) and asserts a non-emptygithub-repo:line. A skill with no frontmatter, an absent key, or an empty value is rejected with a clear message. Stays jq/grep-only — no newyqdependency, matching the rest of the guard.validate-manifests.test.sh: the fixturemake_pluginnow writes a SKILL.md with provenance frontmatter (happy path stays green), plus three new FAIL cases — stripped provenance, no frontmatter, and an emptygithub-repovalue — so a future refactor that weakens the guard fails the self-test.AGENTS.md: documents the new check (structure comment + the "Skills come from upstream" section).Validation
shellcheckclean on both scripts.validate-manifests.test.sh: 24 passed, 0 failed.validate-manifests.shagainst the live repo: exit 0, all currently-bundled skills carry provenance.Behaviour-preserving for every conformant skill; only catches a genuinely hand-authored / provenance-stripped one. Opened as a draft for your promotion.